Floating point constant expression #189
Conversation
| @@ -10,6 +10,7 @@ | |||
|
|
|||
| namespace Cesium.CodeGen.Tests; | |||
|
|
|||
| [UseCulture("")] | |||
There was a problem hiding this comment.
I'm curious why explicit control of the culture is needed here? Isn't we should write code which runs on all cultures in same way.
There was a problem hiding this comment.
Instruction.ToString() uses the culture of the thread, which can lead to different results on different machines and fail tests
There was a problem hiding this comment.
Cool that you submit jbevain/cecil#870 to fix underlying root cause. Would be interesting to know how it would be responded.
| @@ -34,6 +34,7 @@ private static IConstant GetConstant(Ast.ConstantExpression expression) | |||
| CTokenType.IntLiteral => new IntegerConstant(constant.Text), | |||
| CTokenType.CharLiteral => new CharConstant(constant.Text), | |||
| CTokenType.StringLiteral => new StringConstant(constant), | |||
| CTokenType.FloatLiteral => new FloatConstant(constant.Text), //TODO: Add DoubleConstant | |||
There was a problem hiding this comment.
As per C11 clause 6.4.4.2, “An unsuffixed floating constant has type double. If suffixed by the letter f or F, it has type float. If suffixed by the letter l or L, it has type long double.“
So I think we can distinguish between float, double and long double trivially.
There was a problem hiding this comment.
In current implementation we don't have suffix support. I have added a todo comment to get back to it when it is supported
There was a problem hiding this comment.
Then probably would be good to have Double by default, at least that would be correct C behaviour, and if we miss float support, because of f suffix missing, then let's add float support later one.
Regarding missing suffix support, is this on us or on Yoakke?
There was a problem hiding this comment.
Suffix support is on us, because I want us to have a separate lexer eventually. Not sure if Yoakke already supports the suffices.
| @@ -71,6 +71,7 @@ int main() | |||
| [Fact] public Task Parameter1Get() => DoTest("int foo(int x){ return x + 1; }"); | |||
| [Fact] public Task Parameter5Get() => DoTest("int foo(int a, int b, int c, int d, int e){ return e + 1; }"); | |||
| [Fact] public Task CharConstTest() => DoTest("int main() { char x = '\\t'; return 42; }"); | |||
| [Fact] public Task FloatConstTest() => DoTest("int main() { float x = 1.5; return 42; }"); | |||
There was a problem hiding this comment.
I think here would be good to have
[Fact] public Task DoubleConstTest() => DoTest("int main() { double x = 1.5; return 42; }");
I think naming is from when Float was default. When we really implement float we need to change name anyway.
There was a problem hiding this comment.
Yeah, I forgot about it
Cesium.CodeGen.Tests/verified/CodeGenMethodTests.FloatConstTest.verified.txt
Outdated
Show resolved
Hide resolved
| @@ -34,6 +34,7 @@ private static IConstant GetConstant(Ast.ConstantExpression expression) | |||
| CTokenType.IntLiteral => new IntegerConstant(constant.Text), | |||
| CTokenType.CharLiteral => new CharConstant(constant.Text), | |||
| CTokenType.StringLiteral => new StringConstant(constant), | |||
| CTokenType.FloatLiteral => new FloatConstant(constant.Text), //TODO: Add DoubleConstant | |||
There was a problem hiding this comment.
Suffix support is on us, because I want us to have a separate lexer eventually. Not sure if Yoakke already supports the suffices.
Add UseInvariantCultureAttribute to CodeGenTestBase
Fantoom
force-pushed
the
floatingpointconst
branch
from
eced66e to
70ca721
Compare
|
I've made an improvement of removing The follow-up issue is #243. |
TODO[F]:
float